-
Notifications
You must be signed in to change notification settings - Fork 341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(adapters): overlapping segs with labelmap images #1815
base: main
Are you sure you want to change the base?
feat(adapters): overlapping segs with labelmap images #1815
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I don't think we're on the right track here. The adapter should return |
I was really hoping you would point to specific parts of the code and ask for specific changes instead of abstract ones. I'm not following what you're asking me to do. |
So basically the idea is to have this work. const { labelMapImages } =
await Cornerstone3D.Segmentation.createFromDICOMSegBuffer(
referenceImageIds,
arrayBuffer,
{
metadataProvider: metaData,
skipOverlapping: false
}
); Then we do csToolsSegmentation.addSegmentations([
{
segmentationId,
representation: {
type: cornerstoneTools.Enums.SegmentationRepresentations
.Labelmap,
data: {
imageIds: labelMapImages.flat()
}
}
}
]); The key files that need to be changed are Here const derivedImageId = getCurrentLabelmapImageIdForViewport(
viewportId,
representation.segmentationId
); as you see it only get one labelmapimageId for viewport => but we have changed that so that it can have multiple for one segmentationId (overlapping) and then later on in that file viewport.addImages([
{
imageId: derivedImageId,
representationUID: `${segmentationId}-${SegmentationRepresentations.Labelmap}`,
callback: ({ imageActor }) => {
imageActor.getMapper().setInputData(imageData);
},
},
]); you see we only add one derivedImageId to the viewport (since we only had support for one slice -> one imageId -> per labelmap) but now we want to support multiple so we need to loop over derivedImageIds for that slice and then add them one by one which should work Hope this helps |
3f0ab31
to
3113419
Compare
713eb66
to
ec8b82a
Compare
…at-overlapping-segs
@sedghi is there a timeline for feedback on this? We are blocked by this PR. |
I've given my review to pedro, seems like he is on vacation? |
@sedghi as asked on slack, here is the table of comparison for the skipOverlapping flag (master vs this branch): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements support for overlapping segmentations by duplicating planar data buffer arrays instead of merging them as before. Key changes include:
- Introducing a new function to merge segmentation data without information loss.
- Refactoring various methods and tests to support multiple labelmap image IDs per segmentation (e.g., replacing getCurrentLabelmapImageIdForViewport with getCurrentLabelmapImageIdsForViewport).
- Adjusting helper functions, event listeners, and examples to correctly handle the new overlapping segments use case.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/adapters/test/compactMergeSegData.jest.js | Unit tests for the new merging function are added and updated. |
packages/adapters/src/adapters/Cornerstone3D/Segmentation/compactMergeSegData.ts | Implements the merging logic without information loss for overlapping segmentation arrays. |
packages/tools/src/stateManagement/segmentation/helpers/getSegmentationActor.ts | Adds helper support to retrieve multiple labelmap actor entries. |
packages/tools/src/stateManagement/segmentation/getCurrentLabelmapImageIdForViewport.ts | Introduces a new function for handling multiple labelmap image IDs. |
packages/tools/src/stateManagement/segmentation/SegmentationStateManager.ts | Refactors state handling to support multiple image IDs using a newly introduced mapping and key generation. |
Remaining files in packages/adapters and packages/tools | Update references and examples to use the new overlapping segments support throughout the codebase. |
Comments suppressed due to low confidence (3)
packages/tools/src/eventListeners/segmentation/imageChangeEventListener.ts:206
- The updated representationUID format now includes the derived image ID. Ensure that all parts of the system expecting the previous format are updated to handle this new naming pattern consistently.
representationUID: `${segmentationId}-${SegmentationRepresentations.Labelmap}-${derivedImage.imageId}`,
packages/adapters/src/adapters/Cornerstone3D/Segmentation/generateToolState.ts:56
- The removal of the skipOverlapping parameter in the configuration object should be reviewed carefully to ensure that overlapping segments are still handled as expected, and all dependent code and documentation are updated accordingly.
{ metadataProvider, tolerance = 1e-3 }
packages/tools/src/stateManagement/segmentation/SegmentationStateManager.ts:626
- [nitpick] The usage of 'referenceImageId' for generating map keys is central to supporting overlapping segments. Verify that this change is consistently propagated in all state management functions and that it does not conflict with previous assumptions about current image IDs.
const referenceImageId = stackViewport.getCurrentImageId();
Context
In order to address OHIF/Viewers#3496 we're taking a two steps approach:
1 - chopping the volume data buffer array into planar data buffer arrays
2 - adopt some strategy to duplicate these planar data buffer arrays when we get overlapping segs
This PR aims to cover the second step.
Changes & Results
Before - loss of information:

After - overlapping segs maintained:

Testing
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-api
to update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment